Skip to content

Conversation

hodimbokom
Copy link
Contributor

@hodimbokom hodimbokom commented Sep 9, 2025

Description

With vueCompilerOptions.strictTemplates: true, adding any event handler to RouterLink (e.g. @focus) triggers a TS error, even though the handler works at runtime (see #2484).

Solution

  • Make RouterLink generic over custom to preserve anchor event/attrs when custom !== true and exclude them when custom === true.
  • Add Omit<AnchorHTMLAttributes, 'href'> so href stays disallowed (we derive it from to).
  • No runtime changes, only types.

Tests

  • Add dts tests to ensure:

    • event handlers and anchor attrs are allowed when not custom
    • href is not allowed
    • anchor attrs/handlers are disallowed when custom is true
  • All tests pass:

    • pnpm -C packages/router build && pnpm -C packages/router build:dts
    • pnpm -C packages/router test:types
    • pnpm -C packages/router test (coverage/unit/e2e)
  • Locally verified with vue-tsc --noEmit and strictTemplates: true on an SFC.

Links

Closes #2484


Also happy to adjust if maintainers prefer another approach.

Summary by CodeRabbit

  • New Features
    • Improved TypeScript typings for RouterLink to better reflect usage modes: when not using custom, standard anchor attributes (e.g., target, rel) and native events are allowed while href is excluded; when using custom, native anchor events are restricted. No runtime changes.
  • Tests
    • Added type-check tests to validate the updated RouterLink typings and ensure correct allowances and errors across modes.

Copy link

netlify bot commented Sep 9, 2025

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit b0ac394
🔍 Latest deploy log https://app.netlify.com/projects/vue-router/deploys/68e671492b072b0008e0c0db

Copy link
Contributor

coderabbitai bot commented Sep 9, 2025

Walkthrough

RouterLink TypeScript typings were made generic to distinguish prop shapes when custom is set or not; AnchorHTMLAttributes was imported and new internal typed aliases were added. DTS tests were expanded to verify allowed/disallowed anchor attributes and event handlers in both modes. No runtime behavior changed.

Changes

Cohort / File(s) Summary
RouterLink typing changes
packages/router/src/RouterLink.ts
Added AnchorHTMLAttributes import; introduced internal _RouterLinkPropsTypedBase and generic RouterLinkPropsTyped<Custom> types; made _RouterLinkI constructor generic to expose a typed $props surface depending on Custom. No runtime code changes.
DTS tests updated
packages/router/test-dts/components.test-d.tsx
Expanded type-check tests to assert event handlers and anchor attributes are allowed when custom is not set, assert href is disallowed in that mode, and assert certain events (e.g., onFocus) are disallowed when custom is present.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibble on types where link-flowers grow,
Tuning props gently so event-handlers show.
Custom or not, I sort what’s allowed,
Tests hop beside me, tidy and proud.
A rabbit’s small tweak — no runtime to slow. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title precisely describes the key change of allowing event handlers on RouterLink under strictTemplates and references the related issue, succinctly conveying the primary purpose of the pull request.
Linked Issues Check ✅ Passed The pull request updates the RouterLink type definitions to permit anchor event handlers when strictTemplates is enabled, implements a generic parameter to discriminate custom usage, omits href correctly, and adds corresponding dts tests, directly satisfying the requirements of issue #2484.
Out of Scope Changes Check ✅ Passed All modifications are confined to RouterLink type definitions and related type tests, and there are no unrelated or extraneous code changes outside the scope of enabling event handlers under strictTemplates.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
packages/router/src/RouterLink.ts (2)

1-31: Prefer type-only import for AnchorHTMLAttributes to avoid any runtime import risk

Move AnchorHTMLAttributes to a separate type-only import for consistency and to keep bundlers from attempting to load it at runtime.

-import {
+import {
   defineComponent,
   h,
   PropType,
   inject,
   computed,
   reactive,
   unref,
   VNode,
   UnwrapRef,
   VNodeProps,
   AllowedComponentProps,
   ComponentCustomProps,
   getCurrentInstance,
   watchEffect,
   // this is a workaround for https://github.com/microsoft/rushstack/issues/1050
   // this file is meant to be prepended to the generated dist/src/RouterLink.d.ts
   // @ts-ignore
   ComputedRef,
   // @ts-ignore
   DefineComponent,
   // @ts-ignore
   RendererElement,
   // @ts-ignore
   RendererNode,
   // @ts-ignore
   ComponentOptionsMixin,
   MaybeRef,
-  AnchorHTMLAttributes,
 } from 'vue'
+import type { AnchorHTMLAttributes } from 'vue'

369-379: Typing strategy achieves goal; verify behavior with dynamic boolean custom bindings

The conditional props cleanly allow anchor events/attrs when custom !== true and exclude them when custom === true, and href is properly omitted. One edge case: <RouterLink :custom="someBool" ...> (or TSX custom={someBool} where someBool: boolean) may not be assignable to either branch of the union (true vs false|undefined), causing a TS error. If that’s acceptable, consider documenting it; otherwise, we may need a broader strategy (tradeoff: allowing dynamic booleans typically re-allows anchor attrs).

You can extend the dts test with:

// dynamic boolean: today this likely errors (by design). Decide if we want to support it.
const isCustom: boolean = Math.random() > 0.5
// @ts-expect-error: dynamic boolean not supported by narrowed props
expectError(<RouterLink to="/" custom={isCustom} onFocus={() => {}} />)
// explicit false should allow anchor attrs/handlers
expectTypeOf<JSX.Element>(<RouterLink to="/" custom={false} onFocus={() => {}} target="_blank" />)
// explicit true should forbid anchor attrs/handlers
// @ts-expect-error
expectError(<RouterLink to="/" custom={true} onFocus={() => {}} />)
packages/router/test-dts/components.test-d.tsx (1)

30-40: Great coverage; add a couple of assertions for explicit custom={false} and ARIA/data fallthrough

Tests capture the core behaviors. Consider adding:

  • acceptance of custom={false} with events/attrs
  • an ARIA/data attribute to ensure broad fallthrough coverage
// allows when explicitly false
expectTypeOf<JSX.Element>(<RouterLink to="/" custom={false} onFocus={() => {}} target="_blank" />)
// allows ARIA/data attrs
expectTypeOf<JSX.Element>(<RouterLink to="/" aria-label="Home" data-testid="rl" />)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e7ce331 and 9248c9f.

📒 Files selected for processing (2)
  • packages/router/src/RouterLink.ts (2 hunks)
  • packages/router/test-dts/components.test-d.tsx (1 hunks)
🔇 Additional comments (1)
packages/router/src/RouterLink.ts (1)

381-383: Generic constructor change LGTM

This preserves inference in JSX while enabling the conditional props. No runtime impact; aligns with the stated objective.

@posva posva force-pushed the fix/routerlink-strict-templates branch from 9248c9f to b0ac394 Compare October 8, 2025 14:12
Copy link

pkg-pr-new bot commented Oct 8, 2025

Open in StackBlitz

npm i https://pkg.pr.new/vue-router@2548

commit: b0ac394

Copy link

codecov bot commented Oct 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.61%. Comparing base (ccb5e18) to head (b0ac394).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2548   +/-   ##
=======================================
  Coverage   89.61%   89.61%           
=======================================
  Files          46       46           
  Lines        4103     4103           
  Branches     1090     1090           
=======================================
  Hits         3677     3677           
  Misses        421      421           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/router/test-dts/components.test-d.tsx (1)

30-40: LGTM! Comprehensive type tests for RouterLink anchor behavior.

The new tests correctly verify:

  • Event handlers (onFocus, onClick) and anchor attributes (target, rel) are allowed when custom is not set
  • href is always disallowed (derived from to)
  • Event handlers are disallowed when custom={true}

Optional enhancement: Consider adding a test for custom={false} explicitly to verify that anchor attributes are also allowed in that case (not just when custom is omitted):

expectTypeOf<JSX.Element>(
  <RouterLink to="/" custom={false} onFocus={() => {}} />
)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9248c9f and b0ac394.

📒 Files selected for processing (2)
  • packages/router/src/RouterLink.ts (2 hunks)
  • packages/router/test-dts/components.test-d.tsx (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/router/test-dts/components.test-d.tsx (2)
packages/router/src/RouterLink.ts (1)
  • RouterLink (361-361)
packages/router/src/index.ts (1)
  • RouterLink (162-162)
packages/router/src/RouterLink.ts (1)
packages/router/src/index.ts (2)
  • RouterLinkProps (165-165)
  • _RouterLinkI (164-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build
  • GitHub Check: build
🔇 Additional comments (3)
packages/router/src/RouterLink.ts (3)

29-29: LGTM! Correct import for anchor attributes and event handlers.

Importing AnchorHTMLAttributes from Vue provides the complete set of anchor element attributes and event handlers needed for the type enhancement.


363-380: LGTM! Well-designed conditional type for custom prop.

The type definitions correctly implement the generic approach:

  • _RouterLinkPropsTypedBase: Base props combining Vue component props + RouterLinkProps
  • RouterLinkPropsTyped<Custom>: Conditional type that:
    • When Custom extends true: Excludes anchor attributes (custom render mode)
    • Otherwise: Includes Omit<AnchorHTMLAttributes, 'href'> for anchor attributes/handlers
    • Always omits href since it's derived from the to prop

The conditional type correctly handles all scenarios via distributivity:

  • Custom = undefined → allows anchor attributes ✓
  • Custom = false → allows anchor attributes ✓
  • Custom = true → disallows anchor attributes ✓
  • Custom = boolean → union of both branches (correctly permissive) ✓

389-390: LGTM! Generic constructor enables precise type inference.

Making _RouterLinkI generic over Custom with a default of boolean | undefined allows TypeScript to:

  • Infer Custom = undefined when the custom prop is omitted → anchor attributes allowed
  • Infer Custom = true when custom is passed → anchor attributes disallowed

This solves the strictTemplates issue while maintaining type safety and backward compatibility.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@posva posva merged commit 8f35c6d into vuejs:main Oct 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using event handler with RouterLink combined with vueCompilerOptions strictTemplates gives TS error
2 participants